Repository navigation
kvm: refresh the RBD pool when resizing a volume libvirt has not cached - #14623
bhouse-nexthop wants to merge 3 commits into
Conversation
LibvirtResizeVolumeCommandWrapper gets the disk with pool.getPhysicalDisk() and then looks the volume up through libvirt with storageVolLookupByPath() to resize it. Before apache#14188, getPhysicalDisk() went through LibvirtStorageAdaptor.getVolume(), which refreshes the libvirt pool when the volume is not found, so the volume was in libvirt's cache by the time the wrapper looked it up. apache#14188 made getPhysicalDisk() stat RBD images through librbd, so nothing refreshes the pool any more. RBD volumes are created through librbd, not libvirt, so a new volume is not in libvirt's cache on a host until something refreshes the pool there, and resizing it fails with: Storage volume not found: no storage vol with matching path '<pool>/<uuid>' When the lookup by path fails with VIR_ERR_NO_STORAGE_VOL on an RBD pool, refresh the pool and look the volume up once more. That is what getVolume() did for this path before apache#14188, and the refresh only happens when the volume is not cached. Other errors and other pool types are handled as before. Signed-off-by: Brad House <bhouse@nexthop.ai>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14623 +/- ##
=========================================
Coverage 18.32% 18.32%
- Complexity 16644 16653 +9
=========================================
Files 5939 5939
Lines 537768 537799 +31
Branches 65929 65935 +6
=========================================
+ Hits 98551 98571 +20
- Misses 428008 428019 +11
Partials 11209 11209
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Test report: resizing RBD volumes that libvirt has not cached, with no manual pool refreshEnvironment
1. Size-only resize of a fresh VM's ROOT, VM running
On the build without this PR, the same first resize failed with 2. Data volume, created, attached and resized while attached to the running VM
3.
Result: pass.
Not covered
|
Move the lookup by path that refreshes an RBD pool when the volume is not in libvirt's cache out of LibvirtResizeVolumeCommandWrapper and into LibvirtUtilitiesHelper.getStorageVolumeByPath(), and call it from the wrapper. The behaviour and the log messages are unchanged. The tests of the lookup itself move to LibvirtUtilitiesHelperTest. LibvirtComputingResourceTest keeps a resize of an RBD volume that is not in libvirt's cache, through the real helper, and adds a resize that fails when the lookup fails. Signed-off-by: Brad House <bhouse@nexthop.ai>
weizhouapache
left a comment
weizhouapache
left a comment
There was a problem hiding this comment.
code lgtm
not tested yet
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19957 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
I ran this change on a KVM + Ceph RBD test environment (two KVM hosts, two RBD primary storage pools) to check whether the libvirt cache-miss path behaves as the PR describes. Test change. The existing smoke tests Diff of the test change (test/integration/smoke/test_volumes.py)@@ -746,6 +746,8 @@ class TestVolumes(cloudstackTestCase):
"Check if the root volume resized appropriately"
)
+ self.resize_root_volume()
+
# start the vm if it is on xenserver
if hosts[0].hypervisor == "XenServer":
@@ -977,6 +979,8 @@ class TestVolumes(cloudstackTestCase):
"Check if the data volume resized appropriately"
)
+ self.resize_root_volume()
+
# start the vm if it is on xenserver
if hosts[0].hypervisor == "XenServer":
@@ -984,6 +988,28 @@ class TestVolumes(cloudstackTestCase):
time.sleep(30)
return
+ def resize_root_volume(self):
+ """Grow the ROOT volume of the VM by one GB, on any primary storage type."""
+ root = self.wait_for_attributes_and_return_root_vol()
+ new_size_gb = int(math.ceil(float(root.size) / (1024 ** 3))) + 1
+ self.debug("Resize ROOT volume (ID: %s) to %d GB" % (root.id, new_size_gb))
+
+ cmd = resizeVolume.resizeVolumeCmd()
+ cmd.id = root.id
+ cmd.size = new_size_gb
+ self.apiClient.resizeVolume(cmd)
+
+ def root_resized():
+ vols = Volume.list(self.apiClient, id=root.id, listall=True)
+ if isinstance(vols, list) and len(vols) > 0:
+ vol = vols[0]
+ if int(vol.size) == new_size_gb * (1024 ** 3) and vol.state == 'Ready':
+ return True, vol
+ return False, None
+
+ res, _ = wait_until(5, 36, root_resized)
+ self.assertTrue(res, "Check if the ROOT volume resized appropriately")
+
def wait_for_attributes_and_return_root_vol(self):
def checkVolumeResponse():Reproduction. I created a data volume, attached it to a running VM on one host (which makes the agent create the RBD image through librbd), then resized it right away. Right after the attach, Results.
Notes. The refresh runs on the pool lookup, so it is limited to the RBD pool and the one lookup that misses. The unpatched runs show that the refresh is needed, and the patched runs show that it recovers from the miss. I have not tested a non-RBD pool, a volume that is already in libvirt's cache, or a shrink. |
|
@bhouse-nexthop |
…ed (PR 14623) Cherry-pick of the PR apache#14623 changes to the resize path, applied on 4.23.0-ceph. The VM snapshot XML tests from the PR are left out since they are not on this branch.
test_08_resize_volume and test_12_resize_volume_with_only_size_parameter only resized DATADISK volumes. Both now also grow the VM's ROOT volume by 1 GB while it runs and wait for the new size and the Ready state. The step runs on any primary storage type, so on Ceph RBD it covers the libvirt pool-cache miss this PR fixes. PowerFlex rounds sizes up to a multiple of 8 GB, which the expected size accounts for, as in test_12. Suggested by Wei Zhou. Signed-off-by: Brad House <bhouse@nexthop.ai>
boring-cyborg
Bot
added
component:integration-test
Python
|
@weizhouapache thanks for testing this on RBD, and for the repro numbers. Done in d18b117: One change from your diff: PowerFlex rounds volume sizes up to a multiple of 8 GB, so the expected size is rounded the same way there, as |
|
[SF] Trillian test result (tid-17530)
|
Description
Resizing a volume on RBD primary storage fails on KVM when the host's libvirt has not cached the image in its view of the pool. A freshly deployed VM's ROOT volume is the usual case:
This is a regression from #14188.
Fixes #14622
Why it happens
LibvirtResizeVolumeCommandWrappergets the disk withpool.getPhysicalDisk(), then looks the volume up through libvirt withconn.storageVolLookupByPath(path)to callStorageVol.resize().getPhysicalDisk()went throughLibvirtStorageAdaptor.getVolume(), which refreshes the libvirt pool on a miss. The volume was therefore in libvirt's cache by the time the wrapper looked it up.getPhysicalDisk()stat RBD images through librbd, so nothing refreshes the pool any more.Fix
When
storageVolLookupByPath()fails withVIR_ERR_NO_STORAGE_VOLon an RBD pool, refresh that pool and look the volume up once more.getVolume()did for this path before kvm: look RBD volumes up through librbd instead of refreshing the whole pool #14188. The refresh only happens when the volume is not cached.StorageVol.resize(), then the resize script, which for RBD isNOTIFYONLYand runsvirsh blockresizeon a running VM.I looked at two other fixes:
LibvirtStorageAdaptoralready does for create, clone and delete. That avoids the refresh entirely, but it replaces the resize itself (shrink handling, the image open, error reporting), so it is a bigger change for a regression fix. It would make a reasonable follow-up.LibvirtStorageAdaptor.getVolume(). It looks the volume up by name on the libvirt pool object and refreshes on any error, so it would change the lookup for volumes that are cached, too.Other call sites
I checked the KVM plugin for other places that rely on
getPhysicalDisk()having refreshed the libvirt pool. There are none:LibvirtStorageAdaptor.deletePhysicalDisk()callsgetVolume()itself, which still refreshes on a miss.LibvirtMigrateCommandWrapper.deleteLocalVolume()usesstorageVolLookupByPath(), but only for local files.Not changed here
execute()already callspool.refresh()after every resize, before it reads the new size back. For RBD that refresh no longer affects the result, since the size now comes from librbd. On a cache miss, this PR adds a second refresh. Dropping the trailing one for RBD is a separate change.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Unit tests. A live lab retest will follow.
LibvirtComputingResourceTestpasses (273 tests, checkstyle on). The new tests are:testResizeVolumeCommandRbdVolumeNotInLibvirtPoolCache: the first lookup throws not found on an RBD pool. The resize refreshes the pool, finds the volume, resizes it, then runs the notify-only script, and answers success with the new size.testResizeVolumeCommandRbdVolumeNotFoundAfterRefreshandtestResizeVolumeCommandRbdVolumeRetriedAfterFailedRefresh: the volume is still missing after the refresh, whether or not the refresh itself worked. There is exactly one refresh and one retry, then the resize fails.testResizeVolumeCommandRbdOtherLookupErrorDoesNotRefreshandtestResizeVolumeCommandRbdLookupErrorWithoutCodeDoesNotRefresh: any other lookup error on RBD fails without a refresh.testResizeVolumeCommandNonRbdVolumeNotFoundDoesNotRefresh: a missing volume on NFS fails without a refresh, as before.Without the fix, the first three of these fail. The other three pass with or without it; they check that nothing else changed.